Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support formatting macro calls with simple brace delimited arguments #6327

Closed
wants to merge 1 commit into from
Closed

Support formatting macro calls with simple brace delimited arguments #6327

wants to merge 1 commit into from

Conversation

johnislarry
Copy link

@johnislarry johnislarry commented Sep 16, 2024 •
edited
Loading

Rustfmt gives up when it sees a macro with a brace-delimited argument. This PR implements formatting for a subset of these: key-value maps. For example:

params! {
  "a" => "b",
    "c" => "d",
}

now has its indentation fixed:

params! {
  "a" => "b",
  "c" => "d",
}

If the macro argument can't be parsed as key-value pairs, then it falls back to the old behavior.

I added a couple test cases and cargo test passes locally.

This is my first rustfmt PR so I hope it's in the right direction - please let me know what I need to do to get this merged!

@ytmimi
Copy link
Contributor

ytmimi commented Sep 16, 2024

Thanks for the PR, but I'm going to close this. At the moment it's an intentional decision to not format brace delimited macros.

@ytmimi ytmimi closed this Sep 16, 2024
@johnislarry
Copy link
Author

@ytmimi Sincerely, let me say a big "thank you!" from me and (I am sure) everybody else working in a Rust codebase.

I understand being an OSS maintainer can be a difficult and thankless job. A well-maintained formatter is a difference-maker for Rust's ergonomics. Thank you!

I want to take 60 seconds to sell you on this PR.

A quick search (pasted below) shows over 25% of open issues pertain to macro formatting. Many are over a year old.

Macro formatting is a common pain point for your users.

There are special cases already cooked into rustfmt for paren and bracket-delimited macros.
And for lazy_static!.

Generic formatting inside macro invocations is hard, but there is an 80/20 of value in supporting common patterns (like vec![], lazy_static!, params! { x => y } etc.)

It won't format everything, but this PR smoothes a particularly sharp corner in the UX.

I know you have more context on why this is intentionally un-addressed. I don't want to speculate too much...

Could you please explain why?

Whatever outcome with this PR, you are doing valuable work - I look forward to seeing rustfmt continue to grow!

$ gh issue list -R rust-lang/rustfmt --search "macro"

Showing 30 of 119 issues in rust-lang/rustfmt that match your search

ID     TITLE                                                                                                                            LABELS                                              UPDATED
#6299  Defining generics in a macro inside macro causes infinite indentation                                                            bug, a-macros                                       about 17 days ago
#8     format macros                                                                                                                    fun!, a-macros                                      about 4 days ago
#6233  Errantly formatting multiline macro                                                                                              poor-formatting, a-macros                           about 9 days ago
#6297  Incorrectly adds space in macro parameters                                                                                                                                           about 17 days ago
#6300  Incorrectly and non-idempotently modifies `..` in macro                                                                          bug, a-comments, a-macros                           about 16 days ago
#6131  Strange formatting of `macro_rules!` macro                                                                                       poor-formatting, a-macros                           about 3 months ago
#6230  Dioxius macros confuse rustfmt                                                                                                   needs-mcve                                          about 2 months ago
#6287  Formatting inside the `lazy_static!` macro did not work when the macro is used with the crate name: `lazy_static::lazy_static!`  p-low, a-macros                                     about 27 days ago
#3705  Allow manually-specifying which macros should be formatted and how                                                               p-low, feature-request, a-macros                    about 10 days ago
#5254  Option: format named macro usage with standard module/function/... formatter                                                     feature-request, a-macros                           about 10 days ago
#4442  rusfmt hangs when using cfg-if macro                                                                                             bug                                                 about 1 month ago
#5928  ICE formatting macro                                                                                                             a-macros                                            about 11 months ago
#5656  let proc-macro's provide a formatting hook for rustfmt                                                                           p-low, feature-request, a-macros                    about 10 days ago
#5905  rustfmt breaks macros involving $                                                                                                bug, a-macros                                       about 8 months ago
#5526  rustfmt removes double colons in macro                                                                                           bug, a-macros                                       about 4 months ago
#3355  [unstable option] format_macro_bodies                                                                                            unstable option                                     about 8 months ago
#3406  Special-case quote! macro                                                                                                        p-low, feature-request, a-macros                    about 2 years ago
#5984  `format_macro_matchers` unusual behavior with commas                                                                             poor-formatting, only-with-option, a-macros         about 5 months ago
#5481  Semicolon-containing non-{} macro invocations inside a macro_rules! are indented incorrectly                                     poor-formatting, p-low, a-macros                    about 2 years ago
#6037  Incomplete `macro_rules` definition is deleted                                                                                   bug, a-macros                                       about 7 months ago
#5959  rustfmt removes parenthesis in macro call                                                                                        bug                                                 about 8 months ago
#3354  [unstable option] format_macro_matchers                                                                                          unstable option                                     about 1 year ago
#5346  [unstable option] `skip_macro_invocations`                                                                                       unstable option                                     about 2 years ago
#4116  [format_macro_matchers] Closure syntax                                                                                           poor-formatting, only-with-option, p-low, a-macros  about 2 years ago
#3960  format_macro_matchers deletes comments                                                                                           bug, only-with-option, p-low, a-macros              about 2 years ago
#5210  [unstable option] `format_asm_macro`                                                                                             unstable option                                     about 2 years ago
#1539  Format MacroDef (macros 2.0)                                                                                                     p-medium, a-macros                                  about 2 years ago
#2905  Format macro calls (not definitions)                                                                                             poor-formatting, a-macros                           about 6 years ago
#6161  perpetually increasing indentation                                                                                               bug, a-macros                                       about 4 months ago
#6097  No way to pass multiple values to list-like `--config` options                                                                   only-with-option                                    about 6 months ago

@ytmimi
Copy link
Contributor

ytmimi commented Sep 17, 2024

@johnislarry Thanks for the kind words, it really means a lot.

I agree that macro formatting is a common pain point, and it's also one of the trickier things rustfmt has to try and handle. Apart from some special casing, rustfmt can only generally format macro calls if the tokens inside those macro calls parse as valid rust code. Macros are allowed to contain any valid rust tokens, which might not necessarily form valid rust syntax, so rustfmt often runs into issues when attempting to parse those tokens. There are also cases where parsing succeeds but it's ambiguous and then rustfmt ends up butchering the formatting (#5709 comes to mind).

To combat some of those issues, rustfmt special cases brace delimited macros {} as an escape hatch for macro formatting and leaves the formatting details up to the developer. At the moment I'm not ready to change that default, but it's certainly something that could be revisited in the future.

@johnislarry
Copy link
Author

@ytmimi I hear what you are saying. Formatting macro calls makes rustfmt a harder engineering problem than (for example) prettier.

Sometimes difficulty (or lack of perfection) is used as an excuse to delay a solution. And issues can go unresolved for years due to this mindset.

I can certainly relate to this.

This PR preserves the brace-delimited escape hatch. It only adds an extra case for (e => e,)+ style syntax.

Respectfully, I want to ask:

Why are you not ready to change this default?

@ytmimi
Copy link
Contributor

ytmimi commented Sep 17, 2024

This PR preserves the brace-delimited escape hatch (emphasis mine)

I don't think that's the case based on the example you provided in the description nor does this hold for the test cases in the PR. You're formatting macros that use braces {}.

Why are you not ready to change this default?

Personally, I'm a fan of the flexibility that the escape hatch of not formatting {} delimited macros affords. Given the challenges around formatting macros, it's nice to have an option that lets users dictate macro formatting.

@johnislarry
Copy link
Author

You're formatting macros that use braces {}

This PR falls back to the original behavior, except for when the macro body is this (e => e,)+ style syntax (which is the example in the description).

There are many other existing test cases (that still pass) where brace-delimited macros remain un-formatted.

I apologize for not being clear about this - maybe this change was perceived as bigger than it is?

This is where the original behavior is used as a fallback: https://github.com/rust-lang/rustfmt/pull/6327/files#diff-315c02cd05738da173861537577d159833f70f79cfda8cd7cf1a0d7a28ace31bR368-R374

Most brace-delimited macros will still take this fallback code path.

Personally, I'm a fan of the flexibility that the escape hatch affords

I can appreciate that this approach makes rustfmt simpler to implement. The problem is that it makes it a less-effective code formatter. (Defined by % of lines where it gives up)

I have to add: I really appreciate your willingness to engage in a discussion about this!

@ytmimi
Copy link
Contributor

ytmimi commented Sep 18, 2024

This PR falls back to the original behavior, except for when the macro body is this (e => e,)+ style syntax (which is the example in the description).

Sorry, but that doesn't change the fact that this formats {} delimited macros. I'm not a fan of the inconsistency that would create, where some {} delimited macros are formatted and others aren't.

As I mentioned above, the current behavior around {} delimited macros is intentional and that's not something we're going to change right now.

@johnislarry
Copy link
Author

Understood - I hope some day there's a better solution here. Good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnislarry @ytmimi @rustbot